Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Internal] Query: Fixes occasional hang while querying using partial partition key against a sub-partitioned container #4359

Conversation

adityasa
Copy link
Contributor

@adityasa adityasa commented Mar 20, 2024

Pull Request Template

Description

This change contains the fix for occasional hang that can occur while querying using partition partition key against a sub-partitioned container. Let's assume a container with partition key definition "/id", "/value1", "/value2". Unlike single-key partition definition, for hierarchical partitions, partition splits occurs such that for a certain value of "/id" (say 2), more than one partition contains the data for this (partial) partition value. When a query is issued with partial partition key ("/id" = 2 in this case), query enumerator (QueryPartitionRangePageAsyncEnumerator L50) attempts to query a feedrange based on this partial value that isn't fully contained inside a single physical partition in the backend. Since such a range cannot be resolved to any single physical partition, enumerator receives a GoneException from the underlying stack. The GoneException is caught by upstream (CrossPartitionRangePageAsyncEnumerator L114) which attempts to resolve the physical partition ranges by consulting the routing map. This is typically a no-op since the GoneException isn't received because of a change in state of the backend.
This doesn't actually help since CrossPartitionRangePageAsyncEnumerator attempts to locate FeedRangeEpk (without the partial partition key, which honors partition boundary) inside a physical partition range - which always succeeds and further downstream (RequestInvokerHandler.ResolveFeedRangeBasedOnPrefixContainerAsync and RequestInvokerHandler L244) ignores this FeedRangeEpk and instead queries using partial partition key (FeedRangePartitionKey) which spans across the partition boundary. The later continues to cause another GoneException and cycle repeats.

This fix limits the epk range for QueryPartitionRangePageAsyncEnumerator such that it does not span across the single physical partition range. It also uses FeedRangeEpk to represent this new partition range, which causes downstream to no longer perform extraneous operations (such as ResolveFeedRangeBasedOnPrefixContainerAsync).

Several parts of the code are suspect here and a more comprehensive fix may be needed to handle all cases generally, outside this change.

  1. RequestInvokerHandler.ResolveFeedRangeBasedOnPrefixContainerAsync should not happen. Ignoring the FeedRange supplied by the upstream blindly is plain wrong. Instead upstream enumerators etc. should provide explicit range.
  2. At the very least using type of FeedRangeInternal (Epk v/s Partition) to determine extra processing in RequestInvokerHandler should not exist. This should be an explicit action.
  3. It's not clear how filtering occurs in the backend when partial partition key is specified and request lands on a physical partition where data for other logical partitions also exists. We need to replicate this behavior in the test infrastructure in the SDK so that such scenarios can be tested broadly.
  4. Query enumerators should not take a dependency on partition key. Instead partition key -> physical partition resolution should take place upstream before they are instantiated.

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • [] New feature (non-breaking change which adds functionality)
  • [] Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • [] This change requires a documentation update

Closing issues

To automatically close an issue: closes #4326

@adityasa adityasa force-pushed the users/adityasa/ICMHierarchicalPartitionKeyHang branch from 7aae2a0 to 5d4a1a1 Compare March 25, 2024 17:32
@adityasa adityasa marked this pull request as ready for review March 25, 2024 18:21
sboshra
sboshra previously approved these changes Apr 1, 2024
Copy link
Contributor

@sboshra sboshra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@adityasa adityasa force-pushed the users/adityasa/ICMHierarchicalPartitionKeyHang branch from 200cb03 to d23b9b7 Compare April 1, 2024 23:30
@adityasa adityasa added the auto-merge Enables automation to merge PRs label Apr 1, 2024
@neildsh
Copy link
Contributor

neildsh commented Apr 2, 2024

                tryCreatePipelineStage = TryCreateSpecializedDocumentQueryExecutionContext(documentContainer, cosmosQueryContext, inputParameters, targetRanges, containerQueryProperties, partitionedQueryExecutionInfo);

[Nit] Please indent for readability #Resolved


Refers to: Microsoft.Azure.Cosmos/src/Query/Core/Pipeline/CosmosQueryExecutionContextFactory.cs:301 in d23b9b7. [](commit_id = d23b9b7, deletion_comment = False)

@neildsh
Copy link
Contributor

neildsh commented Apr 2, 2024

Regarding (3) in the description, the EPK range headers are used to indicate EPK range filtering to the backend. That, or the query itself can have a filter predicate.
Regarding (4), currently, I dont think query enumerators have a dependency on the partition key #Resolved

@adityasa
Copy link
Contributor Author

adityasa commented Apr 2, 2024

Neil - yes, I learnt about the startEpkRange and endEpkRange headers while working on this change. Do you know if the DocumentResourceContentEnumerator honors this range in the backend for Query (or something else) ?

For 4, I was referring to QueryPartitionRangePageAsyncEnumerator, which uses PartitionKey field for making a decision about FeedRange. #Resolved

Copy link
Contributor

@sboshra sboshra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@microsoft-github-policy-service microsoft-github-policy-service bot merged commit 1106cd7 into master Apr 2, 2024
22 checks passed
@microsoft-github-policy-service microsoft-github-policy-service bot deleted the users/adityasa/ICMHierarchicalPartitionKeyHang branch April 2, 2024 22:51
"EffectiveRangesForPartitionKey should be populated when PK is specified in request options.");
}

foreach (Documents.Routing.Range<String> epkForPartitionKey in
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

String

[Nit][Coding convention] Please use string everywhere

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-merge Enables automation to merge PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feed iterator enters an infinite loop after a physical partition split has occurred
4 participants